Improve agent planning and observability#380
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_726ee705-83fe-4512-b2fc-d095ac6824ac) |
📝 WalkthroughWalkthroughThe agent gateway now supports structured model proposals with shared parsing, bounded tool validation, policy authorization, confirmation handling, policy-derived capability messaging, and expanded audit/dashboard breakdowns. ChangesStructured agent planning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DiscordBot
participant AgentOrchestrator
participant StructuredPlanner
participant ToolRegistry
participant PolicyEngine
participant AuditReporter
DiscordBot->>AgentOrchestrator: submit agent request
AgentOrchestrator->>StructuredPlanner: generate typed proposal
StructuredPlanner-->>AgentOrchestrator: return draft actions
AgentOrchestrator->>ToolRegistry: validate tool names and arguments
ToolRegistry-->>AgentOrchestrator: return validated actions
AgentOrchestrator->>PolicyEngine: authorize actions
PolicyEngine-->>AgentOrchestrator: return execution or confirmation decision
AgentOrchestrator->>AuditReporter: record model, actions, and outcomes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a894cede-e07a-4113-84dd-0a364b332db5) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6028b9a7b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolved_member_agreement = self._plan_member_agreement_from_crm( | ||
| text, | ||
| context, | ||
| planner="deterministic_regex", | ||
| ) |
There was a problem hiding this comment.
When an admin asks something like Create a task to send member agreement to Caleb, this new pre-parse resolver matches the trailing member agreement ... to Caleb phrase before _parse_action sees the create a task intent, so the agent plans a DocuSeal member-agreement submission instead of creating a task. Before this change, member-agreement CRM resolution only ran after the deterministic task parser missed; keep that ordering or explicitly exclude task-creation requests to avoid turning task requests into external agreement sends.
Useful? React with 👍 / 👎.
| "github_issue.search_issues": frozenset({"query", "repository", "state", "limit"}), | ||
| "github_issue.create_issue": frozenset({"title", "repository", "body", "labels"}), | ||
| "crm_read.search_contacts": frozenset({"query", "limit"}), | ||
| "crm_write.update_contact": frozenset({"contact_id", "updates"}), |
There was a problem hiding this comment.
With the structured planner enabled, allowing an arbitrary updates object means a model-proposed crm_write.update_contact can pass any CRM attribute through validation; the executor later applies those keys via contact.set(**updates). For admin requests that the model maps to this tool, this expands the previous deterministic onboarding-state-only behavior into arbitrary CRM mutation after confirmation, so the planner gate should enforce a nested allowlist such as cOnboardingState before accepting the draft.
Useful? React with 👍 / 👎.
| agent_structured_planner_enabled: bool = True | ||
| agent_structured_planner_timeout_seconds: float = 8.0 |
There was a problem hiding this comment.
These new env-backed settings are documented, but .env.example still only lists the older agent normalizer variables. AGENTS.md says to “Update docs and .env.example when introducing new config,” and without sample entries operators cloning or upgrading from the example env won’t see how to disable the structured planner or tune its timeout.
Useful? React with 👍 / 👎.
| for outcome in metadata.get("tool_outcomes") or []: | ||
| if not isinstance(outcome, dict): | ||
| continue | ||
| tool_name = str(outcome.get("tool_name") or "unknown") | ||
| status_label = str(outcome.get("status") or "unknown") |
There was a problem hiding this comment.
This new Tool outcome breakdown is computed from the rows returned by _dashboard_agent_request_report, whose SQL still filters to action = 'agent.request'. Confirmed writes record their actual execution results under agent.confirmation, so most write tools (which require confirmation) will be omitted from these counts and the dashboard will under-report or show zero outcomes for the executions operators need to inspect.
Useful? React with 👍 / 👎.
| - outline_write.invite_user: email string, or contact_id/contact_query for a CRM contact. | ||
| - account_write.create_user_accounts: contact_id string or contact_query string, mailbox_username string. | ||
| - memory_read.get_user_facts: optional user_id string. | ||
| - memory_read.get_project_facts: optional project_id string. |
There was a problem hiding this comment.
The production prompt now tells the model it can draft memory_read.get_project_facts, but the Discord agent context does not populate a trusted project_id, and the tool later rejects both missing project context and arbitrary user-supplied project IDs. For project-memory questions routed through the structured planner, this will pass planning/policy and then fail execution instead of asking for a supported project context or declining the workflow.
Useful? React with 👍 / 👎.
| if {"project:read", "task:create"} & scopes: | ||
| capabilities.append( | ||
| "- Tasks: search a project, create tasks, and update your own tasks." |
There was a problem hiding this comment.
For an Engineer role, PolicyEngine grants project:read but not task:create or task:update_own; this intersection still adds the Tasks capability and says the agent can create and update tasks, even though those requests will be denied. Split task read/write capability text or require the matching write scopes before advertising task creation and updates.
Useful? React with 👍 / 👎.
| "source_type", | ||
| "source_ref", | ||
| "source_excerpt", | ||
| "verification_status", | ||
| "confidence", |
There was a problem hiding this comment.
For structured-planner memory writes, these allowlisted fields let the model set provenance and trust metadata that _remember_memory_fact later persists verbatim. A mistaken or prompt-injected plan can create a remembered fact with forged source_ref or elevated verification_status/confidence after normal confirmation, so these metadata fields should be derived by the backend rather than accepted from planner output.
Useful? React with 👍 / 👎.
| agent_planner_model: str = "accounts/fireworks/models/kimi-k2p6" | ||
| agent_fallback_model: str = "gpt-4.1-mini" | ||
| agent_structured_planner_enabled: bool = True | ||
| agent_structured_planner_timeout_seconds: float = 8.0 |
There was a problem hiding this comment.
With a model provider configured, the backend may spend the full 8 seconds in the structured planner before falling back to deterministic parsing, but the Discord bot's backend request timeout is also 8 seconds. In slow-provider cases the bot can time out and report a gateway failure even though the backend would otherwise recover, so set the planner timeout below the bot/API timeout or raise the outer request timeout.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/five08/agent/evals.py (1)
974-987: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFallback loses the brace-extraction step, can crash on recoverable output.
parse_planner_draftitself extracts the JSON object between the first{and last}before parsing (planner.py lines 241-253). If that extraction succeeds but Pydantic validation then fails (ValidationError, e.g. an unrecognized legacy status label), thisexceptblock re-parses the originalraw_outputwith plainjson.loadsinstead of reusing the extracted substring. For provider output that needed extraction (e.g. wrapped in markdown fences or surrounding prose), this secondjson.loads(raw_output)call raises a new, uncaughtJSONDecodeError— since it's raised from inside theexceptclause, it is not caught by that same handler and propagates to the caller. The eval harness will crash for exactly the recoverable-but-nonstandard outputs this fallback was designed to handle.🐛 Proposed fix — reuse the extracted substring
def _parse_live_planner_json(raw_output: str) -> LivePlannerDraft: try: return LivePlannerDraft.model_validate( parse_planner_draft(raw_output).model_dump(mode="python") ) except (ValidationError, ValueError, json.JSONDecodeError): - payload = json.loads(raw_output) + value = raw_output.strip() + try: + payload = json.loads(value) + except json.JSONDecodeError: + start = value.find("{") + end = value.rfind("}") + if start < 0 or end <= start: + raise + payload = json.loads(value[start : end + 1]) normalized = dict(payload)Consider exporting the extraction step from
planner.pyas a small reusable helper so both call sites stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/five08/agent/evals.py` around lines 974 - 987, _parse_live_planner_json should reuse the brace-extracted JSON when fallback validation fails instead of calling json.loads on the original raw_output. Extract and export a shared helper from planner.py, then use it in both parse_planner_draft and _parse_live_planner_json so wrapped or fenced output remains parseable during fallback normalization.
🧹 Nitpick comments (1)
packages/shared/src/five08/agent/planner.py (1)
170-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated retry-POST block.
The retry-without-
response_formatpath (lines 178-186 and 193-202) duplicates the URL, headers,json,timeout, andverifyarguments almost verbatim. Extracting a small_post_completion(url, headers, payload)helper would remove the duplication and prevent future edits (e.g., adding a header) from being applied to only one of the two call sites.♻️ Proposed refactor
+ def _post(self, base_url: str, api_key: str, payload: dict[str, Any]) -> requests.Response: + return requests.post( + f"{base_url.rstrip('/')}/chat/completions", + headers={ + "Authorization": f"Bearer {api_key}", + "Content-Type": "application/json", + }, + json=payload, + timeout=self.timeout_seconds, + verify=default_ca_bundle_path(), + ) + def plan(self, *, message, context, runtime_config, model_tier): ... - response = requests.post( - f"{base_url.rstrip('/')}/chat/completions", - headers={...}, - json=payload, - timeout=self.timeout_seconds, - verify=default_ca_bundle_path(), - ) + response = self._post(base_url, api_key, payload) if ( _should_retry_without_response_format(response) and "response_format" in payload ): payload.pop("response_format", None) - response = requests.post( - f"{base_url.rstrip('/')}/chat/completions", - headers={...}, - json=payload, - timeout=self.timeout_seconds, - verify=default_ca_bundle_path(), - ) + response = self._post(base_url, api_key, payload)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/five08/agent/planner.py` around lines 170 - 211, Extract the duplicated completion POST logic into a small helper such as _post_completion, accepting the URL, headers, and payload while applying the shared timeout and certificate verification settings. Update both the initial request and the retry in the planner completion flow to call this helper, preserving the existing response_format removal and retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/worker/src/five08/worker/config.py`:
- Around line 64-65: Update agent_structured_planner_timeout_seconds in the
configuration model to use the same positive-value validation as the other
timeout fields, defining it with Field(default=8.0, gt=0) and ensuring the
required Field import is available.
---
Outside diff comments:
In `@packages/shared/src/five08/agent/evals.py`:
- Around line 974-987: _parse_live_planner_json should reuse the brace-extracted
JSON when fallback validation fails instead of calling json.loads on the
original raw_output. Extract and export a shared helper from planner.py, then
use it in both parse_planner_draft and _parse_live_planner_json so wrapped or
fenced output remains parseable during fallback normalization.
---
Nitpick comments:
In `@packages/shared/src/five08/agent/planner.py`:
- Around line 170-211: Extract the duplicated completion POST logic into a small
helper such as _post_completion, accepting the URL, headers, and payload while
applying the shared timeout and certificate verification settings. Update both
the initial request and the retry in the planner completion flow to call this
helper, preserving the existing response_format removal and retry behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36dada46-8516-45fb-baff-8b1d1038438e
📒 Files selected for processing (20)
ENVIRONMENT.mdapps/admin_dashboard/src/main.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-DCDV0WK0.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/agent.pyapps/worker/src/five08/worker/config.pydocs/configuration.mddocs/discord-agent-eval-harness.mdpackages/shared/src/five08/agent/__init__.pypackages/shared/src/five08/agent/evals.pypackages/shared/src/five08/agent/orchestrator.pypackages/shared/src/five08/agent/planner.pypackages/shared/src/five08/agent/tools.pytests/unit/test_agent_cog.pytests/unit/test_agent_gateway.pytests/unit/test_agent_planner.pytests/unit/test_backend_api.py
| agent_structured_planner_enabled: bool = True | ||
| agent_structured_planner_timeout_seconds: float = 8.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add gt=0 validation to agent_structured_planner_timeout_seconds.
Other timeout fields in this class enforce positivity with Field(default=X, gt=0) (e.g., job_lead_classifier_timeout_seconds at line 79, intake_resume_fetch_timeout_seconds at line 112). The new field uses a plain float = 8.0 default, allowing zero or negative values that could cause the planner HTTP call to fail immediately or behave unexpectedly.
🛡️ Proposed fix
- agent_structured_planner_timeout_seconds: float = 8.0
+ agent_structured_planner_timeout_seconds: float = Field(default=8.0, gt=0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent_structured_planner_enabled: bool = True | |
| agent_structured_planner_timeout_seconds: float = 8.0 | |
| agent_structured_planner_enabled: bool = True | |
| agent_structured_planner_timeout_seconds: float = Field(default=8.0, gt=0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/five08/worker/config.py` around lines 64 - 65, Update
agent_structured_planner_timeout_seconds in the configuration model to use the
same positive-value validation as the other timeout fields, defining it with
Field(default=8.0, gt=0) and ensuring the required Field import is available.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0a7f6666-632a-464f-820a-455a94f12ad6) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20725b7c24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if re.search(r"\bcreate\s+(?:a\s+)?task\b", text, re.IGNORECASE): | ||
| return None |
There was a problem hiding this comment.
Fresh evidence beyond the earlier task case: this exclusion only protects create task, but plan() still calls the CRM member-agreement resolver before _parse_action, so requests like Create a GitHub issue to send member agreement to Caleb are intercepted as DocuSeal agreement submissions (or denied for engineers without CRM scopes) instead of following the deterministic GitHub-issue path. Exclude all other supported create/update workflows here, or move this resolver back after deterministic parsing misses.
Useful? React with 👍 / 👎.
| "contract_version": PLANNER_CONTRACT_VERSION, | ||
| "message": message, | ||
| "untrusted_context": render_untrusted_context(context.context_snippets), | ||
| "thread": thread or [], | ||
| "runtime_config": {"github_default_repo": default_repo}, |
There was a problem hiding this comment.
When the structured planner handles task requests with relative dates like “tomorrow” or “next Friday”, this payload gives the model the message and context but no trusted current date, while the planner contract asks for due_date as YYYY-MM-DD. That makes the planner produce stale/wrong ISO dates or non-ISO strings that _optional_date later drops, so confirmations can create tasks with incorrect or missing due dates compared with the deterministic parser’s server-date resolution.
Useful? React with 👍 / 👎.
| if tool_name == "memory_write.remember_fact": | ||
| if not _non_empty_arg(args, "key") or not isinstance( | ||
| args.get("value_json"), dict | ||
| ): | ||
| return "What fact should I remember?" |
There was a problem hiding this comment.
For structured-planner memory writes, a draft with scope_type: "project" only needs key and value_json to pass this clarification gate. Discord agent contexts do not currently populate a trusted project_id, so a Project Manager request like “remember this for this project” can be frozen and confirmed, then fail during execution when _trusted_project_scope_id raises project_id is required in trusted context; mirror the project-read context check before presenting confirmation.
Useful? React with 👍 / 👎.
| if draft.status == "needs_clarification": | ||
| question = draft.clarification_question or "What should I do next?" | ||
| return AgentResponse( | ||
| status="needs_clarification", | ||
| message=question, |
There was a problem hiding this comment.
When the structured planner returns needs_clarification, production returns that question immediately, but the live eval path still falls back to the deterministic parser for parseable task searches. With the planner enabled, a model response like the eval-covered Who should I look up? for Show tasks for project Atlas matching onboarding will pass live eval via fallback while production asks a clarification instead of executing the deterministic search, masking planner regressions.
Useful? React with 👍 / 👎.
| AgentToolAction( | ||
| tool_name=draft_action.tool_name, | ||
| arguments=draft_action.arguments, | ||
| summary=draft_action.summary, |
There was a problem hiding this comment.
For live-model plans this copies the model-supplied summary directly into the frozen plan, and the Discord confirmation text renders human_summary rather than a deterministic view of the tool name and arguments. If the model is confused or influenced by untrusted context, an admin can be asked to confirm a benign-looking summary for a different validated write action, weakening the human confirmation gate; generate the summary from the validated action/arguments instead.
Useful? React with 👍 / 👎.
| clarification_question=clarification, | ||
| ) | ||
| return self._response_for_actions( | ||
| actions=actions, |
There was a problem hiding this comment.
This accepts every allowlisted model action as one confirmation plan, so if the planner decomposes “create 508 accounts” into standalone mail_write.create_mailbox, sso_write.create_user, and outline_write.invite_user instead of the composite account_write.create_user_accounts, confirmation executes the standalone tools without the composite preflight/CRM email update semantics and continues after failures. Enforce the composite account tool or reject dependent multi-write planner drafts before confirmation.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
The prior live planner eval measured a richer planner than the production gateway used. Production now consumes the same structured planning contract while preserving deterministic and legacy-normalizer fallbacks for unavailable providers.
Validation
uv run --locked pytest -q tests/unit/test_agent_planner.py tests/unit/test_agent_gateway.py tests/unit/test_agent_evals.py tests/unit/test_agent_cog.py tests/unit/test_backend_api.py tests/unit/test_worker_config.pyuv run --locked python scripts/agent_eval.py --suite canonical --model primary --no-env-file --json— 25 passed, 0 failed, 1 accepted known failure./scripts/lint.sh./scripts/pyrefly.shbun run typecheck && bun run lint && bun run test && bun run buildinapps/admin_dashboardA full Python-suite run reached 1,882 passing tests before an order-dependent PyO3/Pydantic crash in pre-existing expected-validation tests; each affected test passes in isolation.
Note
Medium Risk
Changes the live agent planning entry point and reporting/audit semantics for Discord workflows; mitigated by documented fallbacks and bounded planner timeouts, but misconfiguration could affect routing or dashboard metrics.
Overview
Aligns the production Discord agent gateway with the shared proposal-only structured planner by injecting
OpenAICompatibleAgentPlanner.from_settingsintoAgentOrchestrator, alongside the existing intent normalizer fallback path.Adds
AGENT_STRUCTURED_PLANNER_ENABLEDandAGENT_STRUCTURED_PLANNER_TIMEOUT_SECONDS(documented to stay underAGENT_API_TIMEOUT_SECONDS) and updates ENVIRONMENT.md to describe structured planning first, with deterministic parsing and legacy normalization as fallbacks.Observability: agent request and confirmation audit metadata now records model tier/source, planned action names, and per-tool outcomes. The dashboard agent report aggregates model, action, and tool outcome counts, limits summary totals to
agent.requestevents, and unions recentagent.confirmationrows into the report query. The admin UI surfaces the new breakdown dimensions. Unsupported-user copy is shortened to "I could not map that to a supported workflow."Reviewed by Cursor Bugbot for commit 20725b7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes